Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mitigate uncaught type error in media_source_engine #5069

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

martinstark
Copy link
Contributor

@martinstark martinstark commented Mar 8, 2023

Closes #4903

Does not fix the underlying root cause of the init function being called on a closed or ended MediaSource, but attempts to mitigate the throwing of an uncaught TypeError.

Thoughts on a proper fix:

Would it make sense to rework mediaSourceOpen_ to await sourceopen, or reject/return false when it isn't, can't, or won't open? This would improve control flow by moving the responsibility of handling the MediaSource state to the caller.

For illustrative purposes:

    this.mediaSourceOpen_ = () => {
      // there is no media source, so it cannot be open
      if (!this.mediaSource_) {
        return Promise.resolve(false);
      }

      // MediaSource was previously open, but
      // MediaSource.endOfStream() has been called
      if (this.mediaSource_.readyState === 'ended') {
        return Promise.resolve(false);
      }

      // it's open, go ahead
      if (this.mediaSource_.readyState === 'open') {
        return Promise.resolve(true);
      }

      // the source is 'closed': not currently attached to a media element
      // it could either open, or the player could close before it opens
      return new Promise((resolve) => {
        // handle when source attaches and is opened
        this.eventManager_.listenOnce(
            this.mediaSource_, 'sourceopen', () => {
              try {
                // ensure not destroyed after async action
                this.destroyer_.ensureNotDestroyed();

                // source is now open!
                resolve(true);
              } catch (_) {
                // player destroyed while waiting
                resolve(false);
              }
            });

        // handle when source is never attached and never opens
        this.eventManager_.listenOnce(
            mediaSource, 'sourceended', () => resolve(false));
        /** The above listener is invalid, this cannot happen.
        *   
        *   Calling mediaSource.endOfStream() when it is not attached to a video
        *   element will throw:
        *
        *   > Uncaught DOMException: An attempt was made to use an object that
        *   > is not, or is no longer, usable
        *
        *   which is, interestingly, another uncaught error we're seeing from
        *   Shaka in our logs.
        * 
        *  This promise would instead need to listen to the player closing down to
        *  reject in cases where 'sourceopen' never triggers, to not leave it dangling.
        **/
      });
    };


  async init(streamsByType, sequenceMode=false,
      manifestType=shaka.media.ManifestParser.UNKNOWN) {
    const ContentType = shaka.util.ManifestParserUtils.ContentType;

    const isOpen = await this.mediaSourceOpen_;
    
    if (isOpen) {
      // ...
    } else {
      // ... assert that this should not happen(?)
    };

@martinstark
Copy link
Contributor Author

Aside: I've got both a personal and corporate CLA, is there a way to choose which one is to apply when creating a PR?

@avelad avelad added type: bug Something isn't working correctly priority: P2 Smaller impact or easy workaround labels Mar 8, 2023
@avelad avelad added this to the v4.4 milestone Mar 8, 2023
@avelad avelad requested review from joeyparrish and theodab March 8, 2023 12:59
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2023

Incremental code coverage: 56.25%

@martinstark martinstark force-pushed the guard-type-error branch 3 times, most recently from e7892d9 to 05a1abc Compare March 8, 2023 14:29
@avelad avelad requested a review from littlespex March 13, 2023 17:50
@avelad avelad merged commit e19fa80 into shaka-project:main Mar 14, 2023
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to execute 'addSourceBuffer' on 'MediaSource': The MediaSource's readyState is not 'open'.
3 participants